Skip to content

fix(pt_expt): reuse the stored min_nbor_dist and batch the neighbor statistics - #5956

Open
yckbz wants to merge 9 commits into
deepmodeling:masterfrom
yckbz:fix-pt-expt-min-nbor-dist
Open

fix(pt_expt): reuse the stored min_nbor_dist and batch the neighbor statistics#5956
yckbz wants to merge 9 commits into
deepmodeling:masterfrom
yckbz:fix-pt-expt-min-nbor-dist

Conversation

@yckbz

@yckbz yckbz commented Aug 4, 2026

Copy link
Copy Markdown

dp --pt-expt compress recomputes the minimal neighbor distance from the
training data on every run even when the model already carries it, and the
recomputation sends a whole set to the device at once.

Reading min_nbor_dist from @variables

dp convert-backend stores the value under @variables in model.json — the
location deepmd/pt/utils/serialization.py writes and the PyTorch and Paddle
backends read back. enable_compression looked only at
model.get_min_nbor_dist() and a top-level min_nbor_dist key, so it never
found the value and fell back to a full pass over the training data. It now
checks @variables too, and logs the source:

DEEPMD INFO  Minimal neighbor distance read from the model file (@variables): 1.240097

--recompute-min-nbor-dist forces a recompute, for a model compressed against
a data set other than the one it was trained on.

Batching the neighbor statistics

NeighborStat.iterator passed a whole set to _execute. The intermediate
tensor is [nframes, nloc, nall, 3], and nall is 27·nloc under periodic
boundaries: a 470-frame, 280-atom set allocates ~24 GiB per intermediate with
several live at once, and larger sets do not fit. It now goes through
AutoBatchSize, as the pt, pd, jax and tf backends already do.

Verification

On a 6-system, 1257-frame carbon nanotube data set (A800-80G), min_nbor_dist
is 1.240096678690 whether recomputed with batching (~3 s) or read from
@variables (statistics skipped). The serialized compressed model, tabulated
data included, is identical in both cases and matches what the current code
produces.

Summary by CodeRabbit

  • New Features

    • Added a --recompute-min-nbor-dist option for compression workflows, allowing supported PyTorch Exportable models to recalculate minimum neighbor distances from training data.
    • Added validation requiring a training script when recomputation is enabled.
    • Improved neighbor-statistics processing with memory-bounded batching for large datasets.
    • Preserved computed neighbor-distance information in graph-lowered compressed models.
  • Bug Fixes

    • Improved detection of saved minimum neighbor-distance values across supported model metadata formats.
    • Preserved existing validation behavior when required distance information is unavailable.
    • Improved compression behavior across CPU and GPU execution environments.

yckbz added 2 commits August 4, 2026 12:33
dp convert-backend stores it there, so compress recomputed it from the
training data on every run. Add --recompute-min-nbor-dist to force a
recompute, and log where the value comes from.
Sending a whole set to the device at once needs hundreds of GiB. Use
AutoBatchSize, as the pt, pd, jax and tf backends already do.
Copilot AI lite review requested due to automatic review settings August 4, 2026 04:37
@dosubot dosubot Bot added the bug label Aug 4, 2026
@github-actions github-actions Bot added the Python label Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a42b1c03-4d62-43e6-a7c2-078425b97593

📥 Commits

Reviewing files that changed from the base of the PR and between 12e34ad and cbb5117.

📒 Files selected for processing (6)
  • deepmd/pt_expt/entrypoints/compress.py
  • deepmd/pt_expt/model/spin_model.py
  • deepmd/pt_expt/utils/auto_batch_size.py
  • deepmd/pt_expt/utils/neighbor_stat.py
  • source/tests/pt_expt/model/test_model_compression.py
  • source/tests/pt_expt/test_compress_min_nbor_dist.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • deepmd/pt_expt/entrypoints/compress.py
  • deepmd/pt_expt/utils/neighbor_stat.py

📝 Walkthrough

Walkthrough

The compression command adds a PyTorch Exportable model option to recompute min_nbor_dist from training data. Saved-value lookup covers multiple model locations. Neighbor statistics use device-aware automatic batching. Graph-lower exports preserve the computed value.

Changes

Compression recomputation flow

Layer / File(s) Summary
Batch neighbor statistics
deepmd/pt_expt/utils/auto_batch_size.py, deepmd/pt_expt/utils/neighbor_stat.py, source/tests/pt_expt/test_compress_min_nbor_dist.py
NeighborStat uses a device-aware AutoBatchSize controller to process frame batches. Tests verify CUDA-only batch growth.
Distance selection and recomputation
deepmd/pt_expt/entrypoints/compress.py, deepmd/pt_expt/model/spin_model.py
Compression reads min_nbor_dist from supported locations or recomputes it when requested. Recompute mode requires a training script. SpinModel delegates the property to its backbone. Graph-lower exports include the value.
CLI wiring and validation coverage
deepmd/main.py, deepmd/pt_expt/entrypoints/main.py, source/tests/pt_expt/test_compress_min_nbor_dist.py, source/tests/pt_expt/model/test_model_compression.py
The CLI exposes and forwards the new flag. Tests cover parsing, lookup precedence, missing metadata, export preservation, and model propagation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CompressionCLI
  participant enable_compression
  participant ExportableModel
  participant TrainingData
  participant NeighborStat
  CompressionCLI->>enable_compression: pass recompute_min_nbor_dist
  enable_compression->>ExportableModel: read saved min_nbor_dist
  alt recomputation requested
    enable_compression->>TrainingData: load training script data
    TrainingData->>NeighborStat: provide frames
    NeighborStat-->>enable_compression: computed minimum neighbor distance
  else saved value available
    ExportableModel-->>enable_compression: saved minimum neighbor distance
  end
  enable_compression->>ExportableModel: write min_nbor_dist to graph-lower export
Loading

Possibly related PRs

Suggested reviewers: wanghan-iapcm, njzjz

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the pull request's main changes: reusing stored min_nbor_dist and batching neighbor-statistics computation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
source/tests/pt_expt/test_compress_min_nbor_dist.py (1)

66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Add execution-path tests for forced recomputation.

This test only confirms parser state. Add focused tests that verify enable_compression ignores a saved value, assigns the value returned by UpdateSel.get_min_nbor_dist, and rejects a missing training_script.

As per coding guidelines, use pytest for single test cases instead of the full test suite.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/tests/pt_expt/test_compress_min_nbor_dist.py` around lines 66 - 70,
Extend the compression tests around enable_compression to cover forced
recomputation: verify the saved minimum-neighbor-distance value is ignored, the
result from UpdateSel.get_min_nbor_dist is assigned, and a missing
training_script is rejected. Use focused pytest test cases with mocked
dependencies rather than invoking the full test suite.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@source/tests/pt_expt/test_compress_min_nbor_dist.py`:
- Around line 66-70: Extend the compression tests around enable_compression to
cover forced recomputation: verify the saved minimum-neighbor-distance value is
ignored, the result from UpdateSel.get_min_nbor_dist is assigned, and a missing
training_script is rejected. Use focused pytest test cases with mocked
dependencies rather than invoking the full test suite.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3dae69f2-f343-4c91-bb44-52c1212ea168

📥 Commits

Reviewing files that changed from the base of the PR and between 6330a2f and 110e60b.

📒 Files selected for processing (5)
  • deepmd/main.py
  • deepmd/pt_expt/entrypoints/compress.py
  • deepmd/pt_expt/entrypoints/main.py
  • deepmd/pt_expt/utils/neighbor_stat.py
  • source/tests/pt_expt/test_compress_min_nbor_dist.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the dp --pt-expt compress workflow by avoiding unnecessary recomputation of min_nbor_dist when it is already stored in the model (including under @variables), and by reducing peak device memory use when computing neighbor statistics via frame batching.

Changes:

  • Teach pt_expt compress to reuse min_nbor_dist from the serialized model buffer, the top-level min_nbor_dist key, or @variables, with a new --recompute-min-nbor-dist override flag.
  • Batch NeighborStat.iterator evaluations via AutoBatchSize.execute_all to avoid loading entire datasets onto device at once.
  • Add pytest coverage for the min_nbor_dist read precedence and CLI flag default behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
source/tests/pt_expt/test_compress_min_nbor_dist.py Adds tests for stored min_nbor_dist precedence and the new CLI flag default.
deepmd/pt_expt/utils/neighbor_stat.py Uses AutoBatchSize to batch neighbor-stat execution over frames to reduce device memory pressure.
deepmd/pt_expt/entrypoints/main.py Wires the new recompute_min_nbor_dist flag through to the compress entrypoint.
deepmd/pt_expt/entrypoints/compress.py Adds @variables lookup for min_nbor_dist and supports forced recomputation via a new parameter.
deepmd/main.py Exposes --recompute-min-nbor-dist on the compress CLI parser (scoped in help to pt-expt).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread deepmd/pt_expt/entrypoints/compress.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Ce Liu <lc6866@outlook.com>
Copilot AI review requested due to automatic review settings August 4, 2026 04:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

deepmd/pt_expt/entrypoints/compress.py:43

  • _read_saved_min_nbor_dist assumes model_dict["@variables"] is a dict by doing (model_dict.get("@variables") or {}).get(...). If @variables is present but not a mapping (corrupt/older metadata, or other producers), this will raise AttributeError during compress. Other code paths in the repo defensively guard @variables with isinstance(..., dict) (e.g. pt_expt DPA4 normalization).
        return float(min_nbor_dist), "the model file"
    min_nbor_dist = (model_dict.get("@variables") or {}).get("min_nbor_dist")
    if min_nbor_dist is not None:
        return float(min_nbor_dist), "the model file (@variables)"
    return None, ""


deepmd/main.py:677

  • --recompute-min-nbor-dist is added to the top-level dp compress parser, so it will be accepted for all backends. However, only the PyTorch Exportable backend actually reads/uses recompute_min_nbor_dist; other backends ignore it (e.g. deepmd/pt/entrypoints/main.py and deepmd/jax/entrypoints/main.py don't pass it through). This can mislead users because the CLI will accept the flag but it will have no effect unless the backend is PyTorch Exportable.

Consider validating at dispatch time (or in each backend entrypoint) that this flag is only allowed with the PyTorch Exportable backend, and error out otherwise.

    parser_compress.add_argument(
        "--recompute-min-nbor-dist",
        action="store_true",
        help="(Supported backend: PyTorch Exportable) Ignore the minimal neighbor "
        "distance saved in the model and recompute it from the training data. "
        "Requires -t,--training-script",
    )

Copilot AI review requested due to automatic review settings August 4, 2026 04:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 5, 2026 11:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

deepmd/pt_expt/entrypoints/compress.py:41

  • model_dict.get("@variables") is assumed to be a dict, but if a malformed/older model file stores a truthy non-mapping (e.g. a list/string), (model_dict.get("@variables") or {}).get(...) will raise AttributeError. Since this data comes from disk, it’s safer to guard the type and treat non-dict values as “not present” (or raise a clearer ValueError).
    min_nbor_dist = (model_dict.get("@variables") or {}).get("min_nbor_dist")
    if min_nbor_dist is not None:
        return float(min_nbor_dist), "the model file (@variables)"
    return None, ""

deepmd/pt_expt/utils/neighbor_stat.py:82

  • This change routes neighbor-stat computation through AutoBatchSize.execute_all, but there’s no unit-level regression test to ensure batching is actually invoked (and keeps working) for the pt_expt backend. Consider adding a lightweight test (e.g. monkeypatch AutoBatchSize.execute_all to assert it’s called) so future refactors don’t accidentally revert to whole-set execution.
                minrr2, max_nnei = self.auto_batch_size.execute_all(
                    self._execute,
                    data_set_data["coord"].shape[0],
                    data_set.get_natoms(),
                    data_set_data["coord"],

@iProzd iProzd added the P0 Blocks the DPA4/DPA4C release. label Aug 10, 2026
@njzjz njzjz added this to the v3.2.0 milestone Aug 10, 2026

@njzjz-bot njzjz-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The saved-distance lookup and frame batching address the main performance problem, but three integration gaps remain: graph-lower outputs discard the recovered metadata, virtual-spin wrappers do not apply it to the backbone that performs compression, and explicit CPU execution on CUDA hosts uses a GPU-style expanding batch policy that can again exhaust host memory. The inline comments describe the affected paths and fixes.

Codex quota is about to reset, so I am using the remaining token budget to complete a concentrated review pass over the outstanding PRs.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh

Comment thread deepmd/pt_expt/utils/neighbor_stat.py
Comment thread deepmd/pt_expt/entrypoints/compress.py
Comment thread deepmd/pt_expt/entrypoints/compress.py
yckbz and others added 4 commits August 11, 2026 10:50
DEVICE is CPU whenever DEVICE=cpu is set, even on a CUDA host, where the
CUDA-OOM handler cannot recover from a host OOM. Base the growth policy
on the selected device instead of CUDA availability.
model.serialize() does not carry the runtime buffer, so the graph branch
dropped the value it had just recovered and the next run fell back to -t.
SpinModel.__getattr__ only delegates reads, so the assignment landed on
the wrapper while get_min_nbor_dist and enable_compression kept reading
the backbone.
@yckbz
yckbz requested a review from njzjz-bot August 11, 2026 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug P0 Blocks the DPA4/DPA4C release. Python

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

5 participants